feat: allow using identity external_id as oauth2 subject#4529
feat: allow using identity external_id as oauth2 subject#4529Micaso wants to merge 2 commits intoory:masterfrom
Conversation
|
if I run Also I am quite unsure if I could resolve the issues from the Docker Image Scanners |
jonas-jonas
left a comment
There was a problem hiding this comment.
Awesome, thanks for your work here! I have a few notes.
| headers: | ||
| Authorization: Basic | ||
| override_return_to: true | ||
| use_external_id: true |
There was a problem hiding this comment.
We have the same pattern for the tokenizer, but there the setting is
subject_source: external_id # or id (default)Could you adjust the code here?
There was a problem hiding this comment.
Hi there! Thx for your review and apologies for the delay in getting back to this—I’ve been a bit tied up lately. I changed it like you requested
| if h.d.Config().OAuth2ProviderUseExternalID(ctx) && params.ExternalID != "" { | ||
| subject = params.ExternalID | ||
| } |
There was a problem hiding this comment.
In the tokenizer we return an error if the identity's external_id is unset, and the subject_source is set to external_id.
I think that would be appropriate here, too, as it's otherwise difficult to figure out which ID was used.
Alternatively, we could probably add another claim, that describes the subject_source to the token, WDYT?
There was a problem hiding this comment.
That makes total sense. I agree that consistency with the tokenizer is the best path forward here to avoid confusion over which ID is being used.
I’ve updated the code to return an error if external_id is unset while subject_source is set to external_id. This keeps the behavior predictable across the codebase. Thanks for pointing that out
|
@jonas-jonas is there still something open I should address? |
This change adds a configuration option `oauth2_provider.use_external_id`. When enabled, Kratos will pass the identity's `external_id` as the subject (`sub`) to Ory Hydra during the OAuth2 login flow. If the toggle is enabled but no `external_id` is present on the identity, it falls back to the internal Identity ID (UUID) to ensure continuity. Part of the effort to better integrate external identity mappings. Closes ory#4528
8f5cfcc to
f24cf8e
Compare
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Kratos as Kratos
participant Config as Config
participant Hydra as Hydra
Client->>Kratos: Complete OAuth2 login (challenge)
Kratos->>Config: OAuth2ProviderSubjectSource(ctx)
Config-->>Kratos: "external_id" / "id"
Kratos->>Kratos: Read Identity.ID and Identity.ExternalID
alt subject_source == "external_id"
alt ExternalID non-empty
Kratos->>Hydra: AcceptLoginRequest(subject=ExternalID, params...)
Hydra-->>Kratos: Accepted
else ExternalID empty
Kratos-->>Client: Error (ExternalID required)
end
else subject_source == "id" or empty
Kratos->>Hydra: AcceptLoginRequest(subject=IdentityID, params...)
Hydra-->>Kratos: Accepted
else
Kratos-->>Client: Error (unknown subject_source)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
selfservice/flow/login/hook_external_id_test.go (2)
79-79: Remove no-op call at Line 79.
fakeHydra.Params()is invoked and discarded with no side effects.Suggested cleanup
- fakeHydra.Params() - err = reg.LoginHookExecutor().PostLoginHook(w, r, identity.CredentialsTypePassword.ToUiNodeGroup(), loginFlow, i, sess, "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/flow/login/hook_external_id_test.go` at line 79, Remove the no-op invocation fakeHydra.Params() in the test since its return value is discarded and it has no side effects; locate the call in the test file (look for fakeHydra.Params() in login/hook_external_id_test.go) and delete that statement, then run the unit tests to ensure nothing else depended on its return value.
111-112: Tighten the failure assertion for the missing external ID case.As written, any error satisfies the test; assert the expected reason to make this case precise.
Suggested assertion hardening
- err = reg.LoginHookExecutor().PostLoginHook(w, r, identity.CredentialsTypePassword.ToUiNodeGroup(), loginFlow, iWithoutExtID, sess, "") - require.Error(t, err) + err = reg.LoginHookExecutor().PostLoginHook(w, r, identity.CredentialsTypePassword.ToUiNodeGroup(), loginFlow, iWithoutExtID, sess, "") + require.ErrorContains(t, err, "external ID")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/flow/login/hook_external_id_test.go` around lines 111 - 112, The test in hook_external_id_test.go currently uses a loose require.Error(t, err); tighten it to assert the specific failure by replacing that call with a precise check such as require.ErrorIs(t, err, expectedErr) (using the package sentinel error, e.g., ErrExternalIDNotFound) or require.EqualError(t, err, "expected error message") or assert.Contains(t, err.Error(), "missing external id") so the test verifies the exact reason for failure; locate the failing test by the err variable in hook_external_id_test.go and use the appropriate sentinel error or explicit message from the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@selfservice/flow/login/hook_external_id_test.go`:
- Line 79: Remove the no-op invocation fakeHydra.Params() in the test since its
return value is discarded and it has no side effects; locate the call in the
test file (look for fakeHydra.Params() in login/hook_external_id_test.go) and
delete that statement, then run the unit tests to ensure nothing else depended
on its return value.
- Around line 111-112: The test in hook_external_id_test.go currently uses a
loose require.Error(t, err); tighten it to assert the specific failure by
replacing that call with a precise check such as require.ErrorIs(t, err,
expectedErr) (using the package sentinel error, e.g., ErrExternalIDNotFound) or
require.EqualError(t, err, "expected error message") or assert.Contains(t,
err.Error(), "missing external id") so the test verifies the exact reason for
failure; locate the failing test by the err variable in hook_external_id_test.go
and use the appropriate sentinel error or explicit message from the code under
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a7a0c85-3db9-49bc-8c98-dd5307e87c3e
📒 Files selected for processing (8)
driver/config/config.godriver/config/config_test.godriver/config/stub/.kratos.oauth2_provider.yamlembedx/config.schema.jsonhydra/fake.gohydra/hydra.goselfservice/flow/login/hook.goselfservice/flow/login/hook_external_id_test.go
|
@jonas-jonas — rebased onto latest master and fixed the CodeQL finding. The remaining CI failures ( |
f24cf8e to
e5043f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hydra/fake.go (1)
29-31: Return a copy fromParams().Exposing the backing slice lets callers mutate shared fake state and makes the helper brittle if it is reused across subtests.
♻️ Suggested tweak
func (h *FakeHydra) Params() []AcceptLoginRequestParams { - return h.params + out := make([]AcceptLoginRequestParams, len(h.params)) + copy(out, h.params) + return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hydra/fake.go` around lines 29 - 31, The Params() method on FakeHydra currently returns the backing slice h.params allowing external mutation; change Params() to return a defensive copy by allocating a new slice of len(h.params) and copying h.params into it (e.g., make([]AcceptLoginRequestParams, len(h.params)) + copy or append) so callers receive an independent slice; update the method on the FakeHydra type to perform this copy before returning.selfservice/flow/login/hook_external_id_test.go (1)
47-89: Make the subtests independent.
fakeHydraaccumulates params across cases, so the length check andlastParamslookup depend on shared mutable state. Recreate the fake inside eacht.Run, or add a reset helper; the standalonefakeHydra.Params()call on Line 79 does not clear anything.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/flow/login/hook_external_id_test.go` around lines 47 - 89, The two subtests share mutable fakeHydra state so fakeHydra.Params() accumulates across runs; make each t.Run independent by either instantiating a fresh fakeHydra inside each subtest (or calling a reset method on fakeHydra) before invoking reg.LoginHookExecutor().PostLoginHook, and remove the stray fakeHydra.Params() call that was intended to clear state; ensure assertions in each case (checking fakeHydra.Params() length or using lastParams) operate on the fresh/reset fakeHydra so tests don't depend on prior runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@selfservice/flow/login/hook_external_id_test.go`:
- Around line 91-112: The test currently only checks require.Error(t, err) after
calling reg.LoginHookExecutor().PostLoginHook which can hide unrelated failures;
change the assertion to validate the specific bad-request/validation error for
the external_id subject_source path by asserting the error type and
reason/message contains the expected validation indicator (e.g., "external_id"
or "subject_source") so the test fails only if the hook does not return the
expected validation error; locate the assertion after the PostLoginHook call in
the test case and replace the generic require.Error check with an assertion that
the error is the expected bad-request/validation error (using the project’s
error-type helper or by checking error string/reason).
---
Nitpick comments:
In `@hydra/fake.go`:
- Around line 29-31: The Params() method on FakeHydra currently returns the
backing slice h.params allowing external mutation; change Params() to return a
defensive copy by allocating a new slice of len(h.params) and copying h.params
into it (e.g., make([]AcceptLoginRequestParams, len(h.params)) + copy or append)
so callers receive an independent slice; update the method on the FakeHydra type
to perform this copy before returning.
In `@selfservice/flow/login/hook_external_id_test.go`:
- Around line 47-89: The two subtests share mutable fakeHydra state so
fakeHydra.Params() accumulates across runs; make each t.Run independent by
either instantiating a fresh fakeHydra inside each subtest (or calling a reset
method on fakeHydra) before invoking reg.LoginHookExecutor().PostLoginHook, and
remove the stray fakeHydra.Params() call that was intended to clear state;
ensure assertions in each case (checking fakeHydra.Params() length or using
lastParams) operate on the fresh/reset fakeHydra so tests don't depend on prior
runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e062d920-edf9-45cd-b930-1f44f4ded9a2
📒 Files selected for processing (7)
driver/config/config.godriver/config/config_test.godriver/config/stub/.kratos.oauth2_provider.yamlembedx/config.schema.jsonhydra/fake.gohydra/hydra.goselfservice/flow/login/hook_external_id_test.go
✅ Files skipped from review due to trivial changes (2)
- driver/config/config_test.go
- driver/config/config.go
🚧 Files skipped from review as they are similar to previous changes (3)
- driver/config/stub/.kratos.oauth2_provider.yaml
- hydra/hydra.go
- embedx/config.schema.json
Replace `use_external_id` boolean config with `subject_source` enum to match the existing tokenizer pattern. The new config accepts: - "id" (default): Use identity ID as OAuth2 subject - "external_id": Use identity's external_id as OAuth2 subject Returns an error when `subject_source` is set to "external_id" but the identity's external_id is unset, ensuring predictable behavior and making it easier to identify which ID was used. This aligns the OAuth2 provider configuration with the session tokenizer implementation for consistency across the codebase. Closes ory#4528
e5043f1 to
afad954
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@embedx/config.schema.json`:
- Around line 2289-2294: The schema description for the "subject_source"
property currently claims a hard error when "external_id" is missing; update the
description to reflect the actual fallback behavior (use the identity UUID/"id"
when external_id is unset) so the config contract matches runtime semantics.
Locate the "subject_source" JSON schema entry and replace the description text
to state that "external_id" will be used when present otherwise the system falls
back to the identity "id" (no error), and keep the enum/default values
unchanged.
In `@hydra/fake.go`:
- Around line 53-56: The fake hydra implementation currently returns a hard
error in the "external_id" case when params.ExternalID is empty; change this to
mirror real Hydra's fallback behavior by returning a derived subject instead of
failing — detect if params.ExternalID == "" and then return params.Subject (or
another existing identifier field used as the OAuth2 subject in this fake) as
the fallback value; update the switch/case handling in fake.go (the
"external_id" branch) to use params.ExternalID when present and fall back to
params.Subject (or the equivalent identifier field) when not, rather than
returning herodot.ErrBadRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02d3fa1d-74cc-44f7-a0f9-fbe40020e33d
📒 Files selected for processing (7)
driver/config/config.godriver/config/config_test.godriver/config/stub/.kratos.oauth2_provider.yamlembedx/config.schema.jsonhydra/fake.gohydra/hydra.goselfservice/flow/login/hook_external_id_test.go
✅ Files skipped from review due to trivial changes (3)
- driver/config/stub/.kratos.oauth2_provider.yaml
- driver/config/config_test.go
- driver/config/config.go
🚧 Files skipped from review as they are similar to previous changes (2)
- selfservice/flow/login/hook_external_id_test.go
- hydra/hydra.go
| "subject_source": { | ||
| "title": "Subject source for OAuth2 login", | ||
| "type": "string", | ||
| "enum": ["id", "external_id"], | ||
| "default": "id", | ||
| "description": "Determines which identifier to use as the subject in OAuth2 login requests. Can be either 'id' (identity ID, default) or 'external_id' (identity's external ID). If 'external_id' is selected but not set on the identity, an error will be returned." |
There was a problem hiding this comment.
external_id error semantics conflict with the stated fallback compatibility behavior.
Line 2294 documents a hard error when external_id is missing, but the PR objectives require fallback to identity UUID for backward compatibility. Please align this config contract with fallback behavior.
Suggested schema wording update
- "description": "Determines which identifier to use as the subject in OAuth2 login requests. Can be either 'id' (identity ID, default) or 'external_id' (identity's external ID). If 'external_id' is selected but not set on the identity, an error will be returned."
+ "description": "Determines which identifier to use as the subject in OAuth2 login requests. Can be either 'id' (identity ID, default) or 'external_id' (identity's external ID). If 'external_id' is selected but missing on the identity, Kratos falls back to the identity ID."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "subject_source": { | |
| "title": "Subject source for OAuth2 login", | |
| "type": "string", | |
| "enum": ["id", "external_id"], | |
| "default": "id", | |
| "description": "Determines which identifier to use as the subject in OAuth2 login requests. Can be either 'id' (identity ID, default) or 'external_id' (identity's external ID). If 'external_id' is selected but not set on the identity, an error will be returned." | |
| "subject_source": { | |
| "title": "Subject source for OAuth2 login", | |
| "type": "string", | |
| "enum": ["id", "external_id"], | |
| "default": "id", | |
| "description": "Determines which identifier to use as the subject in OAuth2 login requests. Can be either 'id' (identity ID, default) or 'external_id' (identity's external ID). If 'external_id' is selected but missing on the identity, Kratos falls back to the identity ID." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@embedx/config.schema.json` around lines 2289 - 2294, The schema description
for the "subject_source" property currently claims a hard error when
"external_id" is missing; update the description to reflect the actual fallback
behavior (use the identity UUID/"id" when external_id is unset) so the config
contract matches runtime semantics. Locate the "subject_source" JSON schema
entry and replace the description text to state that "external_id" will be used
when present otherwise the system falls back to the identity "id" (no error),
and keep the enum/default values unchanged.
There was a problem hiding this comment.
The error behavior is intentional — @jonas-jonas explicitly requested in his review that we return an error (rather than fall back) when external_id is missing, to match the tokenizer pattern and keep behavior predictable. The description is correct as-is.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| case "external_id": | ||
| if params.ExternalID == "" { | ||
| return "", herodot.ErrBadRequest.WithReasonf("The identity does not have an external ID set, but it is required for the OAuth2 provider subject.") | ||
| } |
There was a problem hiding this comment.
FakeHydra enforces hard failure instead of fallback for missing external_id.
Line 54-Line 56 rejects requests when external_id is missing, which contradicts the fallback behavior required for compatibility. The fake should mirror fallback behavior so tests validate the intended contract.
Suggested behavior alignment
case "external_id":
- if params.ExternalID == "" {
- return "", herodot.ErrBadRequest.WithReasonf("The identity does not have an external ID set, but it is required for the OAuth2 provider subject.")
- }
+ // Allow missing external_id and fall back to identity ID behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hydra/fake.go` around lines 53 - 56, The fake hydra implementation currently
returns a hard error in the "external_id" case when params.ExternalID is empty;
change this to mirror real Hydra's fallback behavior by returning a derived
subject instead of failing — detect if params.ExternalID == "" and then return
params.Subject (or another existing identifier field used as the OAuth2 subject
in this fake) as the fallback value; update the switch/case handling in fake.go
(the "external_id" branch) to use params.ExternalID when present and fall back
to params.Subject (or the equivalent identifier field) when not, rather than
returning herodot.ErrBadRequest.
There was a problem hiding this comment.
Same reasoning as above — the fake intentionally mirrors the real DefaultHydra behavior, which returns an error when external_id is missing. This is by design per the reviewer's feedback, and the test in hook_external_id_test.go specifically validates this error path.
There was a problem hiding this comment.
@Micaso, you're right — I apologize for the incorrect review comment. Looking at the DefaultHydra implementation in hydra/hydra.go, it also returns herodot.ErrBadRequest when subject_source is "external_id" but ExternalID is empty (no fallback). The FakeHydra correctly mirrors this contract, and the schema description likewise documents that an error is expected in this case. The test in hook_external_id_test.go validating this error path confirms the intended behavior. My suggestion to add a fallback was mistaken.
✏️ Learnings added
Learnt from: Micaso
Repo: ory/kratos PR: 4529
File: hydra/fake.go:53-56
Timestamp: 2026-04-28T09:16:22.325Z
Learning: In the ory/kratos repository, `FakeHydra` in `hydra/fake.go` intentionally mirrors the real `DefaultHydra` behavior: when `SubjectSource` is `"external_id"` and `params.ExternalID` is empty, it returns `herodot.ErrBadRequest` (no fallback to identity ID). This is by design — the schema (`embedx/config.schema.json`) documents that using `external_id` requires the identity to have an external ID set, otherwise an error is expected. The test `TestLoginExecutorWithExternalID` in `selfservice/flow/login/hook_external_id_test.go` validates this error path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Description
This PR introduces the ability to use an identity's
external_idas the OpenID Connectsubject(subclaim) when Ory Kratos acts as the identity provider for Ory Hydra.Previously, Kratos always passed the internal UUID as the subject. For users migrating from legacy systems or integrating with third-party services that rely on specific string identifiers, this new configuration option allows for seamless identity mapping without requiring a custom consent provider middleware.
The logic implements a safe fallback: if the configuration is enabled but an identity does not have an
external_idset, it will continue to use the Kratos Identity UUID to prevent flow breakage.Related issue(s)
Fixes #4528
Checklist
Further Comments
Solution Choice
I implemented this via a new configuration toggle
oauth2_provider.use_external_id. This maintains backward compatibility by defaulting tofalse.The subject selection logic in
hydra/hydra.gofollows this priority:oauth2_provider.use_external_idisfalse: UseIdentity.ID.oauth2_provider.use_external_idistrueANDIdentity.ExternalIDis present: UseIdentity.ExternalID.oauth2_provider.use_external_idistrueANDIdentity.ExternalIDis empty: Fallback toIdentity.ID.Alternatives Considered
Custom Integration Layer: One could bypass the native Kratos-Hydra integration and handle the
acceptLoginRequestmanually in a custom UI/backend. However, this requires users to write and maintain "glue code" for a very common architectural need. Providing this natively simplifies the Ory stack for enterprise migrations.Testing
driver/config/config_test.goto verify schema parsing.hydra/fake.goto capture and verify parameters.PostLoginHookcorrectly extracts theexternal_idfrom the identity.Summary by CodeRabbit
New Features
Configuration
Tests